Make tls optional and add rustls support (#418) #420
Conversation
This change removed the required dependency to hyper-tls and openssl. The allow tls, clients will now have to enable either the `native-tls` or `rustls-tls` features. BREAKING: tls isn't enabled by default anymore.
| @@ -20,7 +20,9 @@ keywords = ["metrics", "telemetry", "prometheus"] | |||
| default = ["http-listener", "push-gateway"] | |||
| async-runtime = ["tokio", "hyper"] | |||
| http-listener = ["async-runtime", "hyper/server", "ipnet"] | |||
| push-gateway = ["async-runtime", "hyper/client", "hyper-tls", "tracing"] | |||
| push-gateway = ["async-runtime", "hyper/client", "tracing"] | |||
There was a problem hiding this comment.
When it comes to the core crates (like metrics), I generally err on the side of making users opt-in to additional functionality, but that flips the opposite direction for exporters, where users generally want/expect to get the "full fat" support out of the box.
I'd say we should add another feature called push-gateway-https, which pulls in hyper-rustls, and have the feature be enabled by default.
| native-tls = ["hyper-tls"] | ||
| rustls-tls = ["hyper-rustls"] |
There was a problem hiding this comment.
After doing some reading, it seems like we should probably just switch to using rustls entirely.
I don't think the initial implementor choose to use hyper-tls specifically for the OS-backed/dynamically-linked aspect, and I don't personally care about esoteric SSL/TLS setups or FIPS compliance or any of that... so I'd rather go with a single solution than force users to have to spend time understanding which one they ought to choose.
There was a problem hiding this comment.
I hope you won't mind a somewhat random drive-past, but its generally a good practice for mid-layer crates that sit between TLS and applications to not set TLS policy.
The way to make this work comfortably is:
- at a dependency level consume hyper, or whatever other crates you use, with
default-features-false - have a feature to turn on the push-gateway without specifying the TLS stack to use
- have a different feature to turn on the push-gateway with whatever tls stack you prefer to have by default
Then higher layer crates such as axum-prometheus can depend on your crate, with default-features=false, and the application layer crate using axum-prometheus can specify hyper-tls or hyper-rustls itself.
This change removed the required dependency to hyper-tls and openssl. The allow tls, clients will now have to enable either the
native-tlsorrustls-tlsfeatures.BREAKING: tls isn't enabled by default anymore.